8364816: GetLastError() in os_windows.cpp should not store value to errno #26901
Conversation
|
👋 Welcome back toxaart! A progress list of the required criteria for merging this PR into |
|
@toxaart This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 67 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jsikstro, @dholmes-ora) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Webrevs
|
There was a problem hiding this comment.
Things are never simple. :)
The basic change of not setting errno from GetLastError is good. That is a bug that had to be fixed.
I think your approach for setting errno when callers expect it to be set is also good (but see below). Most callers of os::stat care only about success or failure - not the details of why (which you have recorded in the logging anyway if the user/developer is interested).
You also fixed the aotClassLocation.cpp hack because you should never have to check errno for Windows specific error codes - so that is an extra bonus IMO.
The more general problem of how to map GetLastError to errno when callers expect to check errno, may still remain in other places. That's a separate issue IMO. But I also note that in JDK 8 code we do perform such a mapping where needed (we have more cases to deal with post JDK 8 but it is worth looking at the old code).
However, this has highlighted a giant hole in the way errno is being used: we have cleanup code that will likely overwrite errno before the caller could get to check it! And if cleanup code doesn't then logging code will. This likely affects non-Windows code too - though it may be ErrnoPreserver is being used in enough places there. This seems to be a fairly new problem - I don't see it in JDK 21. I have filed JDK-8366272
I have flagged one thing, in relation to _wopen that needs fixing, but otherwise I think this PR is okay in this form. But then we need the follow up to properly manage errno.
Thanks
Hi, please consider the following changes:
In
os_windows.cppin a few places results returned byGetLastError()are stored toerrno. However,errnohas no relation toGetLastError(), and their ranges are not the same.Results of
GetLastError()should be stored into variables of typeDWORD.The removed section in
src/hotspot/share/cds/aotClassLocation.cppwas relying on values returned byGetLastError()and stored toerrnoinos::stat(). Though the logic was correct, these values should not be stored toerrno. The functionality is preserved by storing a valid valueENOENTtoerrnoinos::stat().Tested in tiers 1 - 3.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26901/head:pull/26901$ git checkout pull/26901Update a local copy of the PR:
$ git checkout pull/26901$ git pull https://git.openjdk.org/jdk.git pull/26901/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26901View PR using the GUI difftool:
$ git pr show -t 26901Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26901.diff
Using Webrev
Link to Webrev Comment